Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure CSP Configuration with Streamlined Settings (backwards incompatible) #219

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

robhudson
Copy link
Member

@robhudson robhudson commented May 1, 2024

This PR introduces a significant update to the django-csp project, focusing on enhancing the coherence between configuration and headers, and aligning with Django's common practices for settings. The CSP settings have been restructured, consolidating them into two primary options for enforced and report-only policies.

Key Changes:

  • Consolidation of Settings: Simplified the settings config by transitioning to a two-setting approach:
    • CONTENT_SECURITY_POLICY: settings for the enforced policy.
    • CONTENT_SECURITY_POLICY_REPORT_ONLY: settings for the report-only policy.

This is a backwards-incompatible change.

While this change may require refactoring for existing users...

  • it consolidates and simplifies django-csp configuration,
  • it ties the CSP configuration more closely with the CSP response headers, simplifying the comprehension of content-security policies,
  • it unlocks the capability to provide both enforceable and report-only headers (discussed in issue Allow enforced CSP and Report-Only at the same time #36).

Feedback and Contributions

We invite our community members to test and provide feedback on the updated settings structure. Your input will help refine django-csp and ensure its compatibility with Django best practices. Documentation enhancements or suggestions are greatly appreciated.

By following this migration guide, you should be able to successfully update your Django project to
use the new dict-based CSP settings format introduced in the latest version of `django-csp`. This
change aligns the package with the latest CSP specification and provides a more organized and
flexible way to configure your Content Security Policy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👨‍🍳 💋

Copy link
Member

@willkg willkg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very much excited about this overhaul. The new configuration model is vastly more ergonomic and easier to work with. Thank you for working on this!

'upgrade-insecure-requests': True,
'report-uri': "/csp-report/",
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I <3 this soooo much more than django-csp 3 configuration.

csp/contrib/rate_limiting.py Outdated Show resolved Hide resolved
csp/contrib/rate_limiting.py Outdated Show resolved Hide resolved
Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not a full review, but some comments around the backwards-incompatibility this introduces)

CHANGES Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/migration-guide.rst Outdated Show resolved Hide resolved
@robhudson robhudson force-pushed the two-dict-based-settings branch 2 times, most recently from d2cd3e2 to 4c12388 Compare May 28, 2024 19:10
@robhudson
Copy link
Member Author

Does anyone also feel like we shouldn't have a top level csp import path? I think current practices for Django apps would suggest something like django_csp instead.

I'm considering changing the import path since we're breaking backwards compatibility anyway but wanted to try to get a poll:

  • 👍 if you think we should do this
  • 👎 if you think this is too much change

warning = (
"You are using django-csp < 4.0 settings. Please update your settings to use the new format.\n"
"See https://django-csp.readthedocs.io/en/latest/migration-guide.html for more information.\n\n"
"We have attempted to build the new CSP config for you based on your current settings:\n\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So good

Copy link
Contributor

@stevejalim stevejalim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ with v v minor comments

Very excited about this - thank you for all the hard (and smart!) work on it 🚀

csp/decorators.py Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
This is a backwards incompatible change.

Also fixes mozilla#139, mozilla#191
@robhudson robhudson merged commit 3413de3 into mozilla:main Jun 6, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants